Skip to content

Conversation

@hippotastic
Copy link
Contributor

This fixes issue #103 by rewriting the broken link check scripts.

Because the previously used broken-link-checker package didn't work as expected for multi-language checks and the API docs did not match the actual behavior of the component, I rolled my own link checking logic based on the popular htmlparser2 library instead.

All link checks are now run locally and are based the output of the astro build command. The script parses the sitemap.xml in the build output directory, builds an index of all pages contained in the sitemap, checks for broken links (including invalid URL hashes) and outputs them in an easy-to-use format grouped by page.

@netlify
Copy link

netlify bot commented Apr 20, 2022

‼️ Deploy request for astro-docs-2 rejected.

Name Link
🔨 Latest commit 8e8d747

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic @hippotastic!

One thing: the docs repo uses pnpm so I think you need to remove package-lock.json and run pnpm i to update pnpm-lock.yaml.

Made a couple of suggestions below, but in general looks great!

@hippotastic
Copy link
Contributor Author

Thank you for the detailed review and great feedback! I updated my PR to address your valid suggestions.

Unfortunately, I found that none of the emojis get displayed correctly in Windows PowerShell and the old terminal, so I replaced them with plain text prefixes and added colors instead to allow faster scanning of the results. I also added a breakdown of the broken link types in the summary as you suggested.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! The plain text solution is actually way better than emojis.

How would you feel about using kleur instead of chalk for the terminal colours? It’s already in the transitive dependencies for the repo and is supposed to be smaller/faster than chalk (with a more or less interchangeable API I believe).

@hippotastic
Copy link
Contributor Author

Done. I usually prefer chalk because kleur is lacking bright colors (they have a long outstanding PR that would solve this, but don't seem to merge it due to confusion with bold, even though I can see a clear difference between bright and bold in my terminal).

However, as I now know that kleur is already being used by Astro, I'm happy to go with that, too.

@delucis
Copy link
Member

delucis commented Apr 21, 2022

Oh interesting! I'm on Mac and just saw bold rather than bright colours so I guess that explains the potential for confusion.

Anyway this looks great! Awesome work!

@hippotastic
Copy link
Contributor Author

@tony-sull Could you please have a look if this can be merged?

It's been reviewed by Chris already and I was informed to mention you now. Hope that's alright!

Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, this is excellent! Big fan of the color coded plaintext output, ran this locally and the output looks excellent and very clear 🥳

@tony-sull tony-sull merged commit ec14d76 into withastro:main Apr 22, 2022
@hippotastic hippotastic deleted the add-new-broken-link-checker branch April 22, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants